Charites v2: Drop support providers, rewrite all to esm module #178
Conversation
|
@smellman I would suggest you split this PR into two:
dropping mapbox support can be a breaking change, it would be nice if you can make a PR which only focus on that. You can increase a major version after removing mapbox if we follow semantic versioning |
|
My main motivation is convert to ESM module from CommonJS and support MapLibre v4, so dropping mapbox support is related of this. |
smellman
changed the title
|
Include yaml-include
smellman
changed the title
There was a problem hiding this comment.
@smellman I commented two things.
furtheremore, is it possible to switch to vite/vitest for better development experience?
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@unvt/charites", | |||
| "version": "0.5.4", | |||
| "version": "2.0.0", | |||
There was a problem hiding this comment.
| "version": "2.0.0", | |
| "version": "1.0.0", |
let's follow semantic versioning and increase a major version since it has breaking changes instead of skipping a version to v2.
There was a problem hiding this comment.
I will release v1.0.0 after this release.
see: #170 (comment) this comment.
| @@ -1,4 +1,4 @@ | |||
| import { test, expect } from '@playwright/test' | |||
| import { test, expect } from '@playwright/test/index.mjs' | |||
There was a problem hiding this comment.
how about dropping support to geolonia too, and make charites only focus on maplibre?
It does not make sense to me to remove mapbox and keep geolonia.
There was a problem hiding this comment.
I don't care about it because Geolonia still uses Charites.
How about you in this comment? > @keichan34 @naogify
There was a problem hiding this comment.
@smellman There are possible 4 cases:
- geolonia is compatible with maplibre, but not mapbox.
- geolonia is compatible with mapbox, but not maplibre.
- geolonia is compatible with both mapbox and maplibre
- geolonia is not compatible with either mapbox or maplibre.
If it is case 1, it does not need to support geolonia. they can use charites as maplibre. No need to differentiate geolonia and maplibre. it is same thing.
if it is case 2, geolonia should be removed since mapbox is removed in this PR.
If it is case 3, geolonia can be removed from charites, but functionality is limited (not supporting mapbox featuer).
If it is case 4, it requires more efforts on third parties to support incompatible map library with maplibre.
There was a problem hiding this comment.
@JinIgarashi geolonia is an extension of maplibre, so that probably falls under case 1 (the parts we add are not really around styling). I'll double check our workflows to see if relying on vanilla maplibre works for us.
There was a problem hiding this comment.
@keichan34 @smellman if geolonia has no difference with maplibre in terms of style specification, I don't think dropping geolonia cause problems. geolonia can use maplibre in charites to style it. the same style from charites should work for geolonia as well.
There was a problem hiding this comment.
@keichan34 How about current status? I want to release until Sunday.
There was a problem hiding this comment.
@smellman Sorry about the late response. Yeah, let's drop geolonia. That leaves just maplibre, so I guess we can get rid of the provider option altogether?
There was a problem hiding this comment.
@keichan34 Thank you for response. Yes, and we can drop $HOME/.charites/config.yml support too.
There was a problem hiding this comment.
@keichan34 @JinIgarashi I remove all provider options and config.yml support.
maplibre version is not hard coded now. |
smellman
changed the title
Thank you for submitting a pull request!
Description
Please describe what you changed briefly.
Type of Pull Request
Verify the followings
mainbranchnpm run buildnpm run lintcharites helpgloballyRefer to CONTRIBUTING.MD for more details.